Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Continuously backup changes and load them when GRIP is launched #822

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

SamCarlberg
Copy link
Member

No description provided.

@JLLeitschuh
Copy link
Member

The only problem I forsee is if the backup file turns out to be corrupted or mal-formed then it makes it impossible to load GRIP without deleting this file.

@codecov-io
Copy link

codecov-io commented Feb 10, 2017

Codecov Report

Merging #822 into master will decrease coverage by -0.47%.
The diff coverage is 12.5%.

@@             Coverage Diff              @@
##             master     #822      +/-   ##
============================================
- Coverage     52.47%   52.01%   -0.47%     
- Complexity     1133     1137       +4     
============================================
  Files           240      241       +1     
  Lines          7708     7784      +76     
  Branches        522      535      +13     
============================================
+ Hits           4045     4049       +4     
- Misses         3479     3549      +70     
- Partials        184      186       +2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 65ab349...3fb9623. Read the comment docs.

@SamCarlberg
Copy link
Member Author

SamCarlberg commented Feb 12, 2017

@JLLeitschuh
Well this is a pain... different save files of the same project aren't guaranteed to be textually identical, even if they're functionally identical. I want to be able to compare a previous save file to the backup to determine which one to load (backup if it's different from the previous save, otherwise the previous save).

@AustinShalit
Copy link
Member

Can you use time modified?

@SamCarlberg
Copy link
Member Author

SamCarlberg commented Feb 12, 2017

Can you use time modified?


Never mind, all changes are automatically saved to the backup files anyway

Copy link
Member

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the overall idea however, this sort of thing could expose an underlying design flaw in GRIP where loading a save loads the file into the active pipeline as it runs. This could put GRIP in a bogus state.

A better solution for GRIP would have been to have XStream read in a data model that then gets copied into what is the running pipeline once everything has been validated correctly.

That being said, I think this could work as it is but I'd want to know it had been tested and was resilient to failures.

Have you tried making the backup something that is bogus (for example half the file isn't there because GRIP is closed mid write of the stream).

Also, adding comments where I indicated would be really helpful.

@@ -61,7 +61,7 @@
*/
private final List<Source> sources = new ArrayList<>();
private final List<Step> steps = new ArrayList<>();
private final Set<Connection> connections = new HashSet<>();
private final Set<Connection> connections = new LinkedHashSet<>();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining the specific use of LinkedHashSet here because clearly it matters now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm actually going to revert this. Just looking at the last modified time now.

@@ -8,6 +8,13 @@
*/
public interface DirtiesSaveEvent {

DirtiesSaveEvent DIRTIES_SAVE_EVENT = new DirtiesSaveEvent() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are fields on interfaces automatically final?

Copy link
Member Author

@SamCarlberg SamCarlberg Feb 22, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fields declared in interfaces are always public static final

Charset.defaultCharset());
} else {
// No project file, delete the last_save file
GripFileManager.LAST_SAVE_FILE.delete();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This logic here doesn't quite make sense in my head. Can you explain this better?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Basically, the LAST_SAVE_FILE stores the full path to the most recently loaded save file. If no file was set, then the LAST_SAVE_FILE is deleted to prevent auto-loading a stale file.


private File lastSaveFile() throws IOException {
List<String> lines = Files.readAllLines(GripFileManager.LAST_SAVE_FILE.toPath());
if (lines.size() == 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why if there is only one line in the file is it considered valid? I'm confused.

private void loadBackup() {
try {
logger.info("Loading backup file");
project.open(GripFileManager.BACKUP_FILE);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If this fails won't the pipeline be in a bogus state and be unusable? this could even crash GRIP because the APP was in a bad state.
If this happens then the user can't open GRIP without deleting the load backup file correct?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welp... guess I'm going to rebase this onto #692, which fixes all of those problems

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or, since #692 is pretty big and breaking... maybe leave this as is and have #692 address this specific problem

} catch (XStreamException | IOException e) {
logger.log(Level.WARNING, "Could not load backup file", e);
}
eventBus.post(DirtiesSaveEvent.DIRTIES_SAVE_EVENT);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this also fire this event if the deserialization fails?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does... That's why the event is after the whole try-catch block.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants